feat: make client port configurable through environment variable #352

0x-r4bbit · 2018-09-11T15:30:05Z

No description provided.

0x-r4bbit · 2018-09-11T15:56:45Z

package.json

@@ -75,7 +75,7 @@
  },
  "scripts": {
    "ui-assets": "copy-aragon-ui-assets -n aragon-ui ./build",
-    "start": "npm run ui-assets && parcel serve src/index.html --port 3000 --out-dir ./build --no-cache",
+    "start": "npm run ui-assets && parcel serve src/index.html --port $ARAGON_CLIENT_PORT || 3000 --out-dir ./build --no-cache",


As @izqui pointed out offline, this expression doesn't actually work in Bash. Only test when $ARAGON_CLIENT_PORT was set.

Will update.

0x-r4bbit · 2018-09-11T16:16:47Z

Bash makes it really hard to read and repetitive as well.. Another way to go about this is using something like if-env. Let me know if you want me to give this a spin.

bpierre · 2018-09-11T16:27:52Z

Maybe we could move the start script to scripts/start.sh? Or even to a node script, for Windows compatibility.

0x-r4bbit · 2018-09-11T16:30:41Z

Oh yea, totally. I like that

sohkai · 2018-09-11T20:42:22Z

It looks like cross-var could be viable too, but it's not widely used and a node script might not be much more effort (and would pave the way for the scripts/start_local.sh to be converted too).

For future reference though, parameter substitution might've been nicer in the script (could just use ${ARAGON_CLIENT_PORT:-3000}).

sohkai · 2018-09-11T21:05:20Z

scripts/start.sh

@@ -0,0 +1,8 @@
+#!/usr/bin/env bash
+
+if [[ ${ARAGON_CLIENT_PORT} ]]; 


Wondering if we should add a REACT_APP prefix, given that all the other env vars use it.

Alternatively, we could change all the other env vars to not use that prefix since we don't need it anymore after moving to parcel.

Wondering if we should add a REACT_APP prefix, given that all the other env vars use it.

I don't have super strong feelings about it, just that I think that bit of information, whether it's a react app or not, is not really necessary in that env variable.

But I'm happy to change it regardless.

Actually, on a second thought, I have to say I probably should've stayed consistent with how env variables have been named in the past. I didn't know REACT_APP_* vars were related to the wrapper client.

0x-r4bbit · 2018-09-12T06:25:03Z

For future reference though, parameter substitution might've been nicer in the script (could just use ${ARAGON_CLIENT_PORT:-3000}).

Ha, that's awesome! Thanks for pointing this out.

I guess I'll convert this script to node then and update the PR

0x-r4bbit · 2018-09-12T07:54:55Z

Alright, this is done. PTAL and let me know if you want to change the env variable.

sohkai

❤️ Looking good!

sohkai · 2018-09-12T10:38:56Z

scripts/start.js

@@ -0,0 +1,7 @@
+const execute = require('child_process').execSync


Should we add the #!/usr/bin/env node here?

Actually, this is already a JS file and executed with node so I don't think this is needed. Unless, of course, you say you want this executable as a plain text file.

Might be nice to include that as an option, in case someone ever does want to.

I'd find it weird to have to use node scripts/start.js rather than just scripts/start.js directly.

Ah sure. I thought this was rather a thing to do when you do indeed have a plain text file (without .js extension). I'll update it :)

sohkai · 2018-09-12T10:41:33Z

scripts/start.js

@@ -0,0 +1,7 @@
+const execute = require('child_process').execSync
+
+const clientPort = process.env.ARAGON_CLIENT_PORT || 3000


I think for now it's easiest to stay consistent and use the REACT_APP prefix, and then we can wholesale remove it from both this repo and aragon-cli in one go after.

0x-r4bbit · 2018-09-12T11:40:09Z

Thanks for the review @sohkai ! I'll update this.

This commit also moves the npm start script into scripts/start for better readability and maintainability.

0x-r4bbit · 2018-09-13T06:59:26Z

@sohkai updated it once more. Notice that I've also dropped the .js extension and went with a plain file instead. This felt more natural to me for this use case and lets you run ./scripts/start.

Let me know if you want to keep the .js extension.

sohkai

Looks good!

izqui · 2018-09-18T15:24:15Z

Good to merge @bpierre?

bpierre · 2018-09-18T15:29:02Z

@izqui I wonder how the execute() would work on WIN environments, but we can see that later. LGTM 👍

0x-r4bbit · 2018-09-18T16:50:08Z

Happy to see this landed! @bpierre I haven't tested this on Windows, but I also couldn't find any active issues on execSync with Windows. Anything in particular you're aware of?

bpierre · 2018-09-18T16:59:49Z

@PascalPrecht Cool! No it’s just that I’m not very familiar with the Windows CLI: I know Unix path separators are accepted, but I was wondering if && and stdio: 'inherit' would work the same way.

0x-r4bbit · 2018-09-18T17:00:54Z

Got it. I guess this needs to be tested then.

0x-r4bbit force-pushed the feat/client-port-env-var branch from da69e66 to 8cdc713 Compare September 11, 2018 15:31

0x-r4bbit mentioned this pull request Sep 11, 2018

Introduce option use custom locally installed Aragon client aragon/aragon-cli#199

Closed

0x-r4bbit commented Sep 11, 2018

View reviewed changes

0x-r4bbit force-pushed the feat/client-port-env-var branch from 8cdc713 to 9dd898c Compare September 11, 2018 16:14

0x-r4bbit force-pushed the feat/client-port-env-var branch from 9dd898c to 52fa04b Compare September 11, 2018 17:40

sohkai reviewed Sep 11, 2018

View reviewed changes

0x-r4bbit force-pushed the feat/client-port-env-var branch from 52fa04b to 30d03f2 Compare September 12, 2018 07:53

sohkai approved these changes Sep 12, 2018

View reviewed changes

0x-r4bbit mentioned this pull request Sep 12, 2018

Ensure configured wrapper port is exposed to Aragon client aragon/aragon-cli#201

Merged

0x-r4bbit force-pushed the feat/client-port-env-var branch from 30d03f2 to 228d329 Compare September 12, 2018 11:46

feat: make client port configurable through environment variable

25211a5

This commit also moves the npm start script into scripts/start for better readability and maintainability.

0x-r4bbit force-pushed the feat/client-port-env-var branch from 228d329 to 25211a5 Compare September 13, 2018 06:58

izqui approved these changes Sep 13, 2018

View reviewed changes

sohkai approved these changes Sep 13, 2018

View reviewed changes

izqui merged commit 66f86ff into aragon:master Sep 18, 2018

0x-r4bbit deleted the feat/client-port-env-var branch September 18, 2018 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make client port configurable through environment variable #352

feat: make client port configurable through environment variable #352

0x-r4bbit commented Sep 11, 2018

0x-r4bbit Sep 11, 2018

0x-r4bbit commented Sep 11, 2018

bpierre commented Sep 11, 2018

0x-r4bbit commented Sep 11, 2018

sohkai commented Sep 11, 2018

sohkai Sep 11, 2018 •

edited

0x-r4bbit Sep 12, 2018

0x-r4bbit Sep 12, 2018

0x-r4bbit commented Sep 12, 2018

0x-r4bbit commented Sep 12, 2018

sohkai left a comment

sohkai Sep 12, 2018

0x-r4bbit Sep 12, 2018

sohkai Sep 12, 2018

0x-r4bbit Sep 12, 2018

sohkai Sep 12, 2018

0x-r4bbit commented Sep 12, 2018

0x-r4bbit commented Sep 13, 2018

sohkai left a comment

izqui commented Sep 18, 2018

bpierre commented Sep 18, 2018

0x-r4bbit commented Sep 18, 2018

bpierre commented Sep 18, 2018

0x-r4bbit commented Sep 18, 2018

		@@ -0,0 +1,8 @@
		#!/usr/bin/env bash

		if [[ ${ARAGON_CLIENT_PORT} ]];

		@@ -0,0 +1,7 @@
		const execute = require('child_process').execSync

		@@ -0,0 +1,7 @@
		const execute = require('child_process').execSync

		const clientPort = process.env.ARAGON_CLIENT_PORT \|\| 3000

feat: make client port configurable through environment variable #352

feat: make client port configurable through environment variable #352

Conversation

0x-r4bbit commented Sep 11, 2018

Choose a reason for hiding this comment

0x-r4bbit commented Sep 11, 2018

bpierre commented Sep 11, 2018

0x-r4bbit commented Sep 11, 2018

sohkai commented Sep 11, 2018

sohkai Sep 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x-r4bbit commented Sep 12, 2018

0x-r4bbit commented Sep 12, 2018

sohkai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x-r4bbit commented Sep 12, 2018

0x-r4bbit commented Sep 13, 2018

sohkai left a comment

Choose a reason for hiding this comment

izqui commented Sep 18, 2018

bpierre commented Sep 18, 2018

0x-r4bbit commented Sep 18, 2018

bpierre commented Sep 18, 2018

0x-r4bbit commented Sep 18, 2018

sohkai Sep 11, 2018 •

edited